-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add traits IntoAscii* to replace (Owned)AsciiCast #17
Conversation
Why do you test on 1.1? I split this and #18 into two pulls because I thought they were independent, But #18 would help in writing tests. When you merge that I'll write those, rebase and squash some of the commits. |
6bc708d
to
eeebd00
Compare
Rebased and: Prepended two semi-related commits because I didn't bother creating new pull requests. In ascii_str:
I thought about adding a method to |
This crate is tested on 1.1 to detect unintended breaking changes, because raising the compilers version is technically a breaking change. That said I don't think it isn't much of a problem to intentionally raise the version, if we test with that particular version. I will check breaking updates using the reverse dependencies feature crates.io provides. I will cherry pick the commit "Implement Default, From<&mut[Ascii]> and As{Ref,Mut}<[Ascii]> for AsciiStr". Thanks! I like the names It's a bit unfortunate that even Thanks for your work and sorry for the delay I caused! |
Unifying traits would require an associated type, so The library still builds on 1.1 (and travis still checks that), It's only the tests that doesn't. In about a month |
feature(ascii) has been stabilized on nightly. In six weeks we can remove "unstable".
5248fa7
to
b0c6b93
Compare
`AsciiCast` requires an explicit lifetime, which doesn't make sense for `Target=Ascii`: `fn by_value<'a,C:AsciiCast<'a,Target=Ascii>>(ch: C) {…}` doesn't compile, (because `'a` doesn't appear anywhere) so you have to borrow ch: `fn by_ref<'a,C:AsciiCast<'a,Target=Ascii>>(c: &'a C) {…}`. But then you have to explicitly borrow it in the caller: `by_ref(& 'a');` which looks bad. The names are also more in line with what std uses: xxx_unchecked() and Into/From.
The only differences are trait and method names.
The error type says where and why conversion failed. Two separate traits to follow std convention. Stops testing on 1.1.0 because the tests doesn't compile there: If we trust Rust to not introduce silent breaking changes, code that passes tests on stable should be correct on older versions as long as it compiles.
Rebased and changed a few things:
|
Since these traits are mainly used for generic conversions I agree we should keep them separate. Four traits isn't too much, I'm simply over-engineering. ;) I think skipping 1.1 for the tests is ok. What is the minimal required version for the tests? Maybe we want to increase the minimum rustc version that is guaranteed to work, advertising that earlier versions may work but are not tested. I have some changes in my queue that require rustc 1.3. Mh, in my opinion dev-dependencies also must follow semver like normal dependencies do. The crate author doesn't know as which kind of dependency another programmer will use it. I agree, great that This branch seems ready to merge, are you polishing the last bits and pieces or can I merge? |
I'm finished. |
AsciiCast
has un-rustic names, and doesn't work well forAscii
(see first commit)Another issue is that the traits aren't implemented for their target type,which breaks the pattern
fn foo<B:Into<Bar>>(b: B)
.Hopefully
AsciiExt
gets stabilized soon.(Then this could be used by
AsciiString.push()
)IntoAsciiStr
/ The third commit is unfinished, just cherry-pick the first two if I don't finish it.I haven't removed changed anything, so this is not a breaking change. The idea is to make a minor release, and then remove (Owned)AsciiCast,
Ascii::from_byte()
and more later.